Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Label): replace isOverflowLabel with variant="overflow" #580

Merged

Conversation

adamviktora
Copy link
Contributor

Closes #556

Copy link
Collaborator

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments below, not a blocker for this PR but worth opening a followup issue for.

code: `import { Label } from '@patternfly/react-core'; <Label someOtherProp />`
}
],
invalid: [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require an update to be made either to the renameProps helper or this rule to be more custom, but we should add an invalid test that checks whether an existing variant prop is replaced or not:

{
  code:   `import { Label } from '@patternfly/react-core'; <Label isOverflowLabel variant="outline" />`,
  output: `import { Label } from '@patternfly/react-core'; <Label variant="overflow" />`,
  errors: [{
      message: `isOverflowLabel prop for Label has been replaced with variant="overflow"`,
      type: "JSXOpeningElement",
      }]
    },

I'd be fine having this be a followup, though, especially since we can't be sure if consumers would be passing both isOverflowLabel and a variant (since the variant shouldn't have any effect for an overflow label I believe).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I think we can do the change right now, I will make this rule more custom

@@ -0,0 +1,17 @@
### label-remove-isOverflowLabel [(#10037)](https://github.com/patternfly/patternfly-react/pull/10037)

isOverflowLabel prop for Label has been replaced with variant="overflow"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to above comment, we should add onto the message here + when an error is thrown: "Running the fix for this rule will replace an existing variant prop." or something along those lines.

Copy link
Collaborator

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. @wise-king-sullyman would you prefer having the open PRs updated to add typing, or do that after alpha when we might have more time to?

@adamviktora adamviktora force-pushed the label_remove_isOverflowLabel branch from d271a3e to 91b73b5 Compare February 22, 2024 09:12
@adamviktora
Copy link
Contributor Author

I updated types on this and the Nav PR to use the types from eslint

@wise-king-sullyman
Copy link
Collaborator

@thatblindgeye ATM I consider typing just a nice to have. After we have the mods for alpha done I would move it up in priority yeah.

The types you've added so far look great though Adam! 🔥

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny non-blocking nit/comment, this looks great though 🚀

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@wise-king-sullyman wise-king-sullyman merged commit 1f856e5 into patternfly:main Feb 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label - isOverflowLabel removed
3 participants